Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Core Data: Replace unnecessary fetch requests for upserting orders and order search results #14128

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from
Open

Core Data: Replace unnecessary fetch requests for upserting orders and order search results #14128

wants to merge 8 commits into from

Conversation

itsmeichigo
Copy link
Contributor

@itsmeichigo itsmeichigo commented Oct 15, 2024 •
edited
Loading

Part of #14090

Description

This PR follows the suggestion in p91TBi-cl2-p2 to clean up unnecessary and expensive fetch requests made in for loops for orders including:

  • Order search results
  • Order items and item taxes
  • Order coupons
  • Order fees
  • Order shipping lines & shipping line taxes
  • Order taxes

Steps to reproduce

  • Log in to a test store with existing products.
  • Create an order by selecting one or more products, and adding coupons, fees, shipping, and taxes. Save the changes.
  • Open the new order from the order list and confirm that all details are correct.
  • Tap the Search button and search for any order. Confirm that the feature works as expected.

Testing information

Tested with simulator iPhone 15 Pro iOS 17.4 and confirmed that order deails and order search work correctly.
All unit tests still pass.

Screenshots

N/A

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

TODO: @itsmeichigo to update the release notes before merging.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

@itsmeichigo itsmeichigo added type: task An internally driven task. feature: order details Related to order details. feature: order list Related to the order list. labels Oct 15, 2024
@itsmeichigo itsmeichigo added this to the 20.8 milestone Oct 15, 2024
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 15, 2024 •
edited
Loading

WooCommerce iOS📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS WooCommerce iOS
Build Numberpr14128-f368915
Version20.7
Bundle IDcom.automattic.alpha.woocommerce
Commitf368915
App Center BuildWooCommerce - Prototype Builds #11179
 Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@itsmeichigo itsmeichigo changed the title Core Data: Unnecessary fetch requests for upserting orders and order search results Core Data: Replace unnecessary fetch requests for upserting orders and order search results Oct 15, 2024
@itsmeichigo itsmeichigo marked this pull request as ready for review October 16, 2024 01:48
@itsmeichigo itsmeichigo mentioned this pull request Oct 16, 2024
Open
@selanthiraiyan selanthiraiyan self-assigned this Oct 17, 2024
selanthiraiyan
selanthiraiyan reviewed Oct 17, 2024
View reviewed changes
Storage/Storage/Tools/StorageType+Extensions.swift Outdated
/// Retrieves the Stored Orders given the IDs.
///
func loadOrders(siteID: Int64, orderIDs: [Int64]) -> [Order] {
let predicate = NSPredicate(format: "orderID in %@", orderIDs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to add the siteID to the predicate? With the current predicate, I am afraid that we will end up loading orders from other sites with the same order ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I updated the predicate in a27533c.

selanthiraiyan
selanthiraiyan reviewed Oct 17, 2024
View reviewed changes
Storage/StorageTests/Tools/StorageTypeExtensionsTests.swift
@@ -66,6 +66,30 @@ final class StorageTypeExtensionsTests: XCTestCase {
XCTAssertEqual(site, storedSite)
}

func test_loadOrders_list_by_siteID_and_orderIDs() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: We could update this test by adding an order with a different site ID other than sampleSiteID to validate that only orders from the given siteID are loaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the test in a27533c.

@itsmeichigo
Copy link
Contributor Author

Thanks @selanthiraiyan for the reviews! I addressed your comment with a commit above, this PR is ready for another look.

@wpmobilebot wpmobilebot modified the milestones: 20.8, 20.9 Oct 18, 2024
@wpmobilebot
Copy link
Collaborator

Version 20.8 has now entered code-freeze, so the milestone of this PR has been updated to 20.9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hafizrahman hafizrahman Awaiting requested review from hafizrahman

@selanthiraiyan selanthiraiyan Awaiting requested review from selanthiraiyan

At least 1 approving review is required to merge this pull request.

Assignees

@selanthiraiyan selanthiraiyan

Labels
feature: order details Related to order details. feature: order list Related to the order list. type: task An internally driven task.
Projects
None yet
Milestone
20.9
Development

Successfully merging this pull request may close these issues.
3 participants
@itsmeichigo @wpmobilebot @selanthiraiyan